-
Notifications
You must be signed in to change notification settings - Fork 104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Accept [package.metadata.raze]
, superseding [raze]
#274
Accept [package.metadata.raze]
, superseding [raze]
#274
Conversation
Cargo sets aside the [`[package.metadata]`][md] namespace for use by third-party tools like `cargo-raze`. By specifying configuration under `[package.metadata.raze]` rather than `[raze]`, we avoid an “unused manifest key: raze” warning on every Cargo command. This patch teaches `cargo-raze` to accept either `[raze]` or the new `[package.metadata.raze]`. In case of conflict, it prints a warning and uses the value of `[package.metadata.raze]`. In the future, we can remove the `[raze]` key entirely if desired. Fixes google#273. Changes outside of `impl/` generated by: ``` git ls-files -z ':!impl' | xargs -0 sed -i 's/\[raze/[package.metadata.raze/' ``` [md]: https://doc.rust-lang.org/cargo/reference/manifest.html#the-metadata-table Test Plan: With this patch, running `./smoke-test.sh` passes both before and after updating all the examples with the above `sed` command. Before the `sed` update, the smoke test prints the deprecation warning, as desired. wchargin-branch: toml-package-metadata-raze wchargin-source: 4932fedcc808b17506aadf4b69f6aa78850fe5c6
wchargin-branch: toml-package-metadata-raze wchargin-source: f2b7fb6fb07bd1f478aa1e8c14f7d623f86f69b3
@@ -370,7 +381,35 @@ pub fn load_settings<T: AsRef<Path>>(cargo_toml_path: T) -> Result<RazeSettings, | |||
return Err(RazeError::Generic(err.to_string())); | |||
} | |||
let mut settings = match toml::from_str::<CargoToml>(&toml_contents) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know if package.metadata.raze
settings can be parsed from cargo metadata
?
Specifically, could something like the CargoMetadataFetcher
be used to load these settings more holistically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, they can. It looks like this:
$ cargo metadata --format-version 1 | jq '.. | .raze? | select(.)'
{
"genmode": "Remote",
"workspace_path": "//third_party/rust",
"crates": {
"crc": {
"1.8.1": {
"gen_buildrs": true
}
},
"indexmap": {
"1.6.0": {
"additional_flags": [
"--cfg=has_std"
]
}
},
"proc-macro2": {
"1.0.24": {
"additional_flags": [
"--cfg=use_proc_macro",
"--cfg=wrap_proc_macro"
]
}
},
"prost-build": {
"0.6.1": {
"gen_buildrs": true
}
},
"thiserror": {
"1.0.21": {
"gen_buildrs": true
}
}
}
}
The path is .packages[].metadata.raze
, so you’d have to pluck it from
the right package.
I haven’t looked at CargoMetadataFetcher
. If you’d prefer that
approach, I can do so (but no promises on when I’ll get around to it).
Note that [raze]
does not show up in cargo metadata
, so as long as
you want to keep both around you would need two parsing paths. So
I might propose a plan of (a) parse both from Cargo.toml
, (b) wait
until comfortable dropping compat, (c) remove CargoToml::raze
,
(d) migrate to parsing from cargo metadata
now that that’s easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was more trying to find out if this information could be parsed all from cargo metadata
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the [package.metadata.raze]
info can; the [raze]
info can’t.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR description makes a compelling case for this change, and this PR keeps the original codepath around so that we don't suddenly break people.
Thanks for making this change, it LGTM.
@acmcarther could you do a release for this change? |
Thanks for the reviews! |
It would be nice to land #127, then I'd be fine releasing 0.7.0 to include this. |
Released in 0.7.0 |
Summary: Version 0.7.0 of `cargo-raze` was just released. Some visible changes: - can now set `[package.metadata.raze]` instead of `[raze]` to avoid a Cargo warning: <google/cargo-raze#274> - build files have been renamed and restructured to be more compliant with Buildifier: <google/cargo-raze#260> If you have an old version installed, run `cargo install cargo-raze` to update to the most recent version. Changes to `third_party/rust/` generated with: ``` (rm -rf third_party/rust/ && cd tensorboard/data/server && cargo raze) ``` The changes to `Cargo.toml` and `third_party/rust/` are independent. Test Plan: Run `(cd tensorboard/data/server && cargo test)` and note that the tests run without an “unused manifest key: raze” warning. Note further that everything still builds with Bazel. wchargin-branch: rust-regen-raze-0.7.0 wchargin-source: c73530a2f7948b4842617ff686f645e66e917d0d
Summary: Version 0.7.0 of `cargo-raze` was just released. Some visible changes: - can now set `[package.metadata.raze]` instead of `[raze]` to avoid a Cargo warning: <google/cargo-raze#274> - build files have been renamed and restructured to be more compliant with Buildifier: <google/cargo-raze#260> If you have an old version installed, run `cargo install cargo-raze` to update to the most recent version. Changes to `third_party/rust/` generated with: ``` (rm -rf third_party/rust/ && cd tensorboard/data/server && cargo raze) ``` The changes to `Cargo.toml` and `third_party/rust/` are independent. Test Plan: Run `(cd tensorboard/data/server && cargo test)` and note that the tests run without an “unused manifest key: raze” warning. Note further that everything still builds with Bazel. wchargin-branch: rust-regen-raze-0.7.0
Cargo sets aside the
[package.metadata]
namespace for use bythird-party tools like
cargo-raze
. By specifying configuration under[package.metadata.raze]
rather than[raze]
, we avoid an “unusedmanifest key: raze” warning on every Cargo command.
This patch teaches
cargo-raze
to accept either[raze]
or the new[package.metadata.raze]
. In case of conflict, it prints a warning anduses the value of
[package.metadata.raze]
. In the future, we canremove the
[raze]
key entirely if desired.Fixes #273.
Changes outside of
impl/
generated by:Test Plan:
With this patch, running
./smoke-test.sh
passes both before and afterupdating all the examples with the above
sed
command. Before thesed
update, the smoke test prints the deprecation warning, as desired.
wchargin-branch: toml-package-metadata-raze